Skip to content
This repository has been archived by the owner on Aug 14, 2024. It is now read-only.

Document session update for dropped events #551

Merged
merged 3 commits into from
Apr 26, 2022
Merged

Conversation

adinauer
Copy link
Member

@adinauer adinauer commented Apr 6, 2022

For filter order and more info please see #537 and getsentry/sentry-java#1916

Do we also want to document order of the filtering mechanisms? If so do we go with the python implementation order as a template?

See #537 and getsentry/sentry-java#1916

Do we also want to document order of the filtering mechanisms? If so do we go with the python implementation order as a template?
@vercel
Copy link

vercel bot commented Apr 6, 2022

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/sentry/develop/9TWSNyqPCK38GQgtEV28RhmBKYJ1
✅ Preview: https://develop-git-adinauer-patch-3.sentry.dev

@adinauer
Copy link
Member Author

adinauer commented Apr 6, 2022

Python checks sampleRate first which probably makes a lot of sense from a performance point of view as I assume this to be the cheapest way of dropping an event. If we want to not update the session for events the developer doesn't care about, it would probably have to be the last check we do. There was a suggestion of moving only the check for ignored error types before the sampling. Maybe this is a good compromise between performance and counting the truth. We can probably safely assume that events dropped due to sampling are events the user would care about unless they are of an ignored error type. For beforeSend and eventProcessor it seems like we are uncertain of why an event is being dropped anyways so it might not be as "wrong" to perform the checks after sampling. This then has the risk of updating the session for events the developer doesn't care about because the event might be dropped by beforeSend or eventProcessor anyways.

@adinauer adinauer requested a review from mitsuhiko April 6, 2022 13:55
adinauer added a commit to getsentry/sentry-python that referenced this pull request Apr 12, 2022
As requested by @mitsuhiko this PR shall serve as basis for discussing the ordering of event drop mechanisms and its implications.

We are planning for `sample_rate` to update the session counts despite dropping an event (see getsentry/develop#551 and getsentry/develop#537). Without changing the order of filtering mechanisms this would mean any event dropped by `sample_rate` would update the session even if it would be dropped by `ignore_errors` which should not update the session counts when dropping an event. By changing the order we would first drop `ignored_errors` and only then check `sample_rate`, so session counts would not be affected in the case mentioned before. The same reasoning could probably be applied to `event_processor` and `before_send` but we don't know why a developer decided to drop an event there. Was it because they don't care about the event (then session should not be updated) or to save quota (session should be updated)? Also these may be more expensive in terms of performance (developers can provide their own implementations for both of those on some SDKs). So moving them before `sample_rate` would execute `before_send` and `event_processor` for every event instead of only doing it for the sampled events.
antonpirker added a commit to getsentry/sentry-python that referenced this pull request Apr 12, 2022
* Change ordering of event drop mechanisms

As requested by @mitsuhiko this PR shall serve as basis for discussing the ordering of event drop mechanisms and its implications.

We are planning for `sample_rate` to update the session counts despite dropping an event (see getsentry/develop#551 and getsentry/develop#537). Without changing the order of filtering mechanisms this would mean any event dropped by `sample_rate` would update the session even if it would be dropped by `ignore_errors` which should not update the session counts when dropping an event. By changing the order we would first drop `ignored_errors` and only then check `sample_rate`, so session counts would not be affected in the case mentioned before. The same reasoning could probably be applied to `event_processor` and `before_send` but we don't know why a developer decided to drop an event there. Was it because they don't care about the event (then session should not be updated) or to save quota (session should be updated)? Also these may be more expensive in terms of performance (developers can provide their own implementations for both of those on some SDKs). So moving them before `sample_rate` would execute `before_send` and `event_processor` for every event instead of only doing it for the sampled events.

Co-authored-by: Anton Pirker <[email protected]>
adinauer added a commit to getsentry/sentry-python that referenced this pull request Apr 14, 2022
We have already merged #1390 where we moved `ignore_errors` before sampling.

Background for the order change:
We want to update the session for dropped events in case the event is dropped by sampling. Events dropped by other mechanisms should not update the session. See getsentry/develop#551

Now we would like to discuss if we can simply move sampling after `before_send` and `event_processor` and update the session right before sampling.
What are implications of changing this?
* How does this affect session count and session crash rate?
* Will this have a negative effect on performance as `before_send` and `event_processor` will now be executed for every event instead of only being executed for sampled events? Developers may have fine tuned their sample rate for a good performance tradeoff and now we change. Also developers can supply their own implementations for both `before_send` and `event_processor` on some SDKs so we have no way of predicting performance I'm afraid.
* We are uncertain why a developer chose to drop an event in `before_send` and `event_processor`:
** Was it because they want to ignore the event - then it shouldn't update the session
** Or was it to save quota - then it should update the session

Please feel free to optimize the code this is just to start the discussion.
sl0thentr0py pushed a commit to getsentry/sentry-python that referenced this pull request Apr 20, 2022
…filter order (#1394)

We want to update the session for dropped events in case the event is dropped by sampling. Events dropped by other mechanisms should not update the session. See getsentry/develop#551
Python now serves as reference implementations. We've recently changed the order there, see getsentry/sentry-python#1394 and getsentry/sentry-python#1390
@vercel
Copy link

vercel bot commented Apr 20, 2022

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated
develop ✅ Ready (Inspect) Visit Preview Apr 25, 2022 at 1:36PM (UTC)

... despite the event being dropped in application mode for certain cases.
@adinauer adinauer merged commit 59ea5e6 into master Apr 26, 2022
@adinauer adinauer deleted the adinauer-patch-3 branch April 26, 2022 10:17
brustolin pushed a commit that referenced this pull request Apr 26, 2022
* Document session update for dropped events

See #537 and getsentry/sentry-java#1916

Do we also want to document order of the filtering mechanisms? If so do we go with the python implementation order as a template?

* Add filter order

Python now serves as reference implementations. We've recently changed the order there, see getsentry/sentry-python#1394 and getsentry/sentry-python#1390

* Session update should be sent...

... despite the event being dropped in application mode for certain cases.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants